Skip to content

Fix segfault on DETACH DELETE under RLS with a function-based edge policy (#2474)#2475

Merged
MuhammadTahaNaveed merged 1 commit into
apache:masterfrom
BenSpex:fix/detach-delete-rls-segfault
Jul 15, 2026
Merged

Fix segfault on DETACH DELETE under RLS with a function-based edge policy (#2474)#2475
MuhammadTahaNaveed merged 1 commit into
apache:masterfrom
BenSpex:fix/detach-delete-rls-segfault

Conversation

@BenSpex

@BenSpex BenSpex commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #2474.

A non-superuser subject to FORCE ROW LEVEL SECURITY crashed the backend with SIGSEGV when running MATCH (n) DETACH DELETE n on a vertex that has a connected edge, if the edge label carried an RLS policy whose USING/WITH CHECK qual invokes a function (e.g. a STABLE tenant/owner accessor). Reproduces on release_PG18_1.7.0 and current master.

Root cause / mechanism

AGE deletes a vertex's connected edges in check_for_connected_edges() (src/backend/executor/cypher_delete.c), which is called from end_cypher_delete() — i.e. during executor shutdown (ExecEndPlan, reached via PortalCleanupExecutorEnd when the portal is dropped). By that point the portal's active snapshot has already been popped, so the active-snapshot stack is empty.

RLS for Cypher DELETE is enforced at the executor level (added in #2309): check_for_connected_edges() compiles the edge label's security quals and evaluates them per candidate edge via check_security_quals()ExecQual(). When the qual calls a SQL-language function, ExecQual() dispatches into fmgr_sql()postquel_start(), which runs the function's query and reads the current snapshot with GetActiveSnapshot(). With no snapshot on the stack that is a NULL-pointer dereference → SIGSEGV. (A cassert build trips the Assert(ActiveSnapshotSet()) in postquel_start first — that assert is exactly the "caller should have ensured a suitable snapshot is active" contract.)

Backtrace (assert build) at the crash site:

Assertion failed: ActiveSnapshotSet(), functions.c
  postquel_start (functions.c)
  fmgr_sql
  ExecQual -> check_security_quals   (cypher_utils.c)
  process_edges_by_index             (cypher_delete.c)
  check_for_connected_edges          (cypher_delete.c)
  end_cypher_delete                  (cypher_delete.c)
  ExecEndCustomScan -> ExecEndPlan -> standard_ExecutorEnd

This explains the full narrowing in the issue:

  • vertex path (process_delete_list) runs during normal execution while a snapshot is active → fine;
  • edge-only DELETE and node-only DETACH DELETE never evaluate an edge qual at teardown → fine;
  • superuser bypasses RLS → fine;
  • a constant-only edge policy never enters fmgr_sql → fine;
  • only DETACH DELETE of an edge-connected vertex under a function-bearing edge policy trips it.

Fix

Ensure an active snapshot for the duration of the connected-edge scan in check_for_connected_edges(). es_snapshot is still valid there (the EState is not torn down until this returns) and is the correct snapshot for reading the edges, so push it if none is active and pop it before returning. The scans themselves already pass es_snapshot explicitly, so this only affects RLS qual evaluation; non-RLS paths are unchanged. On an error path (e.g. an RLS denial raised mid-scan) transaction abort resets the active-snapshot stack, so the unpaired push is cleaned up.

if (!ActiveSnapshotSet())
{
    PushActiveSnapshot(estate->es_snapshot);
    pushed_snapshot = true;
}
...
if (pushed_snapshot)
    PopActiveSnapshot();

Test

Adds a regression to the security suite (PART 11b): an edge-label policy whose qual calls a STABLE SQL function, then DETACH DELETE of an edge-connected vertex as the RLS-bound role. It must delete the vertex and its edge (leaving the other endpoint) instead of crashing. The existing PART 11 uses a constant-only edge policy, which does not exercise the snapshot-dependent function path.

Verification

  • A/B on an identical release-style build (PG18, -O2, --enable-cassert off): unpatched → client backend ... was terminated by signal 11: Segmentation fault; patched → deletes cleanly, no crash.
  • make installcheck against PG18 (--enable-cassert) — all 43 tests pass, including the new case.
  • The issue's function-based repro no longer crashes and deletes correctly.

…licy

A non-superuser subject to row-level security crashed the backend with
SIGSEGV when running DETACH DELETE on a vertex that has a connected edge,
if the edge label carried an RLS policy whose USING/WITH CHECK qual
invokes a function (e.g. a STABLE tenant/owner accessor). Reported in
issue apache#2474; reproduces on 1.7.0 and master.

Mechanism:

AGE deletes a vertex's connected edges in check_for_connected_edges(),
which is called from end_cypher_delete() -- i.e. during executor shutdown
(ExecEndPlan, reached via PortalCleanup -> ExecutorEnd when the portal is
dropped). By that point the portal's active snapshot has already been
popped, so the active-snapshot stack is empty.

RLS for Cypher DELETE is enforced at the executor level (added in apache#2309):
check_for_connected_edges() compiles the edge label's security quals and
evaluates them per candidate edge with check_security_quals() -> ExecQual().
When the qual calls a SQL-language function, ExecQual() dispatches into
fmgr_sql() -> postquel_start(), which runs the function's query and reads
the current snapshot with GetActiveSnapshot(). With no snapshot on the
stack that is a NULL-pointer dereference -> SIGSEGV (an assert build trips
the Assert(ActiveSnapshotSet()) in postquel_start first).

This is why the narrowing in the report holds: the vertex path
(process_delete_list) runs during normal execution while a snapshot is
active; edge-only DELETE and node-only DETACH DELETE never evaluate an
edge qual at teardown; a superuser bypasses RLS; and a constant-only edge
policy never enters fmgr_sql, so only DETACH DELETE of an edge-connected
vertex under a function-bearing edge policy trips it.

Fix:

Ensure an active snapshot for the duration of the connected-edge scan in
check_for_connected_edges(). es_snapshot is still valid there (the EState
is not torn down until this returns) and is the correct snapshot for
reading the edges, so push it if none is active and pop it before return.
The scans themselves already pass es_snapshot explicitly, so this only
affects RLS qual evaluation; non-RLS paths are unchanged. On the error
path (e.g. an RLS denial raised mid-scan) transaction abort resets the
active-snapshot stack, so the unpaired push is cleaned up.

Test:

Adds a regression to the security suite (PART 11b): an edge label policy
whose qual calls a STABLE SQL function, then DETACH DELETE of an
edge-connected vertex as the RLS-bound role. It must delete the vertex and
its edge (leaving the other endpoint) instead of crashing. The existing
PART 11 uses a constant-only edge policy, which does not exercise the
snapshot-dependent function path.

@gregfelice gregfelice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. I reproduced the crash, verified the fix, and reviewed the snapshot handling. The diagnosis in the description is correct and the fix is the right one.

Reproduced

Built current master (9fb7df8) against PostgreSQL 18.4 and ran this PR's new PART 11b case against it. The DETACH DELETE under the function-based edge policy kills the backend:

SET ROLE rls_user1;
SELECT * FROM cypher('rls_graph', $$
    MATCH (p:Person {name: 'DetachFn1'}) DETACH DELETE p
$$) AS (a agtype);
server closed the connection unexpectedly
	This probably means the server terminated abnormally
	before or while processing the request.
connection to server was lost

With this branch applied, the same test passes, and the full 41-test regress suite is green. So the new regress case is a genuine reproducer — it fails on master and passes here — which is exactly what a crash fix needs.

On the fix

Pushing estate->es_snapshot when the active-snapshot stack is empty is the right call, and es_snapshot is the correct snapshot to use — it is what the rest of the statement read the graph with, so connected-edge visibility stays consistent with the vertex scan that selected them.

I specifically checked the failure mode a PushActiveSnapshot/PopActiveSnapshot pair around a loop invites — an early exit between push and pop leaking the snapshot. There is none: check_for_connected_edges() has no return or goto between the two, only a break in an inner loop. The one non-local exit is the pre-existing RLS-denial ereport(ERROR), and the reasoning in the comment holds — AbortTransaction() runs AtAbort_Snapshot(), which resets the active-snapshot stack, so the unpaired push on that path is cleaned up. The if (!ActiveSnapshotSet()) guard also correctly makes this a no-op on any path that already has a snapshot, so nothing changes for the normal-execution callers.

The pushed_snapshot flag rather than an unconditional push/pop is the right shape, for the same reason.

One thing for the release, not for this PR

@jrgemignani — this is a backend SIGSEGV, and PG18/v1.8.0-rc0 is already tagged with artifacts staged in dev/age/PG18/1.8.0.rc0. So this either slips to 1.8.1 or it justifies respinning RC1. That is your call as RM, but I did not want a crash fix to land quietly while an RC is in flight — flagging it explicitly rather than letting the timing decide by default.

Nice write-up on the root cause, @BenSpex. The narrowing in the description (vertex path fine because it runs during normal execution with a snapshot active; edge-only DELETE and node-only DETACH DELETE fine because they never evaluate an edge qual at teardown; superuser fine because RLS is bypassed; constant-only policy fine because it never reaches fmgr_sql) matches what I see in the code, and it is what makes the LANGUAGE sql STABLE accessor in the test the right thing to exercise.

@MuhammadTahaNaveed MuhammadTahaNaveed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@MuhammadTahaNaveed MuhammadTahaNaveed merged commit 2e4b4ad into apache:master Jul 15, 2026
6 checks passed
@gregfelice

Copy link
Copy Markdown
Contributor

@jrgemignani — pulling a few release-adjacent items into one place so they're not scattered across threads.

1. Two PRs are reviewed and ready to merge:

2. Release decision on #2475: this is a backend crash, and PG18/v1.8.0-rc0 is already tagged with artifacts staged in dev/age/PG18/1.8.0.rc0. So it's either a 1.8.1 item or a reason to respin RC1. Your call as RM — I just didn't want a segfault fix landing quietly against an in-flight RC.

3. PR-queue CI was stalled: the workflow runs on every recent contributor PR were sitting in action_required (unapproved), so they showed zero checks. I approved the pending runs across #2468/#2469/#2470/#2475/#2476, so those now have real CI signal. Result of that: #2469 (loader delimiter) is red — new SQL test cases without a regenerated expected file — I left the author a note.

Since I reviewed both of the above, they'll need another committer's hand on the merge button.

@gregfelice

Copy link
Copy Markdown
Contributor

Follow-up now that this is merged (thanks @MuhammadTahaNaveed) — flagging a release-branch gap.

The fix is on master (2e4b4ad) but not on release/PG18/1.8.0. That branch tip is still e43dc1a, which predates this merge, so the staged PG18/v1.8.0-rc0 artifacts do not contain the crash fix. As it stands, 1.8.0 would ship with a backend SIGSEGV that's already fixed on master.

I verified the cherry-pick so the mechanics are a known quantity if you decide to respin:

  • git cherry-pick 2e4b4ad9 onto release/PG18/1.8.0 applies cleanly — no conflicts, exactly the 3 expected files (cypher_delete.c, security.sql, security.out).
  • Builds against PG18, and the full 41-test regress suite passes on the release branch with it applied — including security, which carries the DETACH-DELETE-under-RLS reproducer.

So this is a clean cherry-pick + RC1 respin whenever you're ready. Your call as RM on timing; just didn't want the vote to go out against a branch that's missing a merged crash fix. (Same likely applies to the PG19 release branch — I only verified PG18.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Segfault: DETACH DELETE of an edge-connected vertex under FORCE ROW LEVEL SECURITY as a non-superuser

3 participants